fix(graph): only evaluate outbound edges from completed nodes#1846
Conversation
884d713 to
badbe04
Compare
|
Friendly ping — CI is green, tests pass, ready for review whenever convenient. Happy to address any feedback. Thanks! 🙏 |
badbe04 to
3a47d90
Compare
|
Friendly ping — this branch has been refreshed on the latest upstream base and all current review feedback has been addressed. It should be ready for review whenever you have a chance. Happy to make any further changes quickly. |
|
Friendly ping — rebased on latest and ready for review. Happy to address any feedback! |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
_find_newly_ready_nodes iterated over ALL nodes in the graph after each batch completion, checking every node for readiness. This caused O(all_edges) evaluation instead of O(outbound_edges) and could fire nodes whose actual dependencies had not completed. Now collects candidate nodes from outbound edges of the completed batch only, preventing incorrect out-of-order execution. Fixes strands-agents#685
3a47d90 to
e9a99ed
Compare
|
Rebased onto latest |
|
This PR is approved by @mkmeral ✅ and rebased on latest |
|
Hi @JerryLeam — thanks again for the approval! 🙏 This PR is mergeable and all review feedback has been addressed. Would you be able to trigger CI and merge when you get a chance? Let me know if anything else is needed. |
Bug
After any node completes, the graph evaluates ALL edges in the entire graph instead of only outbound edges from the completed nodes. This causes
O(all_edges)evaluation instead ofO(outbound_edges)and can fire nodes whose actual dependencies haven't completed yet.Reported in: #685
Root cause
_find_newly_ready_nodes()iterates overself.nodes.items()(ALL nodes in the graph) and checks each one against the completed batch. Nodes connected to irrelevant edges could pass the readiness check if their incoming edges happened to match the completed batch.Fix
Replace the all-nodes iteration with a targeted lookup:
This changes the evaluation from
O(all_nodes × incoming_edges)toO(outbound_edges × incoming_edges), and prevents false-positive readiness detection.Testing
test_find_newly_ready_nodes_only_evaluates_outbound_edges: builds a graph with two independent chains (A→B→C, D→E), verifies completing A only readies B (not E), and completing D only readies E (not B or C)Fixes #685